Make the product pop-up images a pluggable system

This question comes through quite regularly, and I think it comes down to Thickbox was just the first thing we tried and integrated. It wouldn't be a bad idea for the system to be pluggable, but I'm not sure if there's an effective way for us to do this in core since the systems rely on different markup (i.e. class vs. rel). If we can't feasibly support one over the other, then I'm not opposed to re-evaluating what we use. I'm postponing this for now unless someone wants to come in with a patch for review.

This patch creates a new hook, uc_image_widgets. it returns an associative array. The key is the internal name e.g. 'thickbox', the value is the description, e.g. 'The ThickBox image widget'.

Each image widget has a corresponding function called uc_image_widget_[widget internal name], i.e. uc_image_widget_thickbox. This function takes as a single argument the index (a.k.a. rel_count in theme_uc_product_image) of the image to display. If this is a single image, pass NULL. The function returns the relevant markup attributes, which are placed inside the link tag generated for each widget.

There's a dialog at admin/store/products/image-widgets which allows the user to select from image widgets that have been defined in such hooks. ThickBox and LightBox are implemented by default, but you can't select them at this dialog unless their modules have been enabled. If this patch is applied, you will have to visit this dialog at least once to enable the widget you'd like to use; the default is None.

I tested both thickbox and lightbox2, both seemed to function. =)

Well, that was quick. ; )

Some feedback on semantics and code placement mostly:

• I'd like to see the setting there worked into an existing settings form. There's already a product settings form that I think this would go fine in.
• Also related to the settings, with a radio select list like this, the title can essentially be what your description is w/ no description necessary... i.e. title => 'Image widget to use when a customer clicks on a product image'. Perhaps the description should then become => 'Third party modules offer pop-up support for viewing full sized product images. Ubercart core accommodates Thickbox and Lightbox2.'
• One last thing here... I think just using the name of the module is enough. So, in your hook_uc_image_widgets(), "Thickbox" and "Lightbox2" communicate enough... the title of the radios element indicates that these are widgets.
• I'm sure there are exceptions to the rule, and they're probably our fault, but hooks are (almost?) universally singular. So, hook_uc_image_widgets() -> hook_uc_image_widget().
• In the default implementation, you use a $plugins array for what you're already referring to as widgets. I don't mind using either term, but go ahead and make it uniform one way or the other (like $plugins -> $widgets).
• Also, to be uniform, I don't see why the key for Lightbox2 shouldn't be lightbox2 since that's the actual name of the module.
• Now that I look a little further, I wonder if we should keep the hook being _uc_image_widget but change the variable related to products to be 'uc_product_image_widget'. This way if we choose in the future we can enable image widget support for other Ubercomponents and have a pattern for naming the variables.
• Last for now, pending an update to the patch - I'd prefer to see the callback function specified in the widget array returned by the hook. i.e. structuring the data returned a little more, like:
  

  'thickbox' => Array (
    'name' => t('Thickbox'),
    'callback' => 'uc_image_widget_thickbox',
  ),
  'lightbox2' => Array (
    'name' => t('Lightbox2'),
    'callback' => 'uc_image_widget_lightbox2',
  )
  

  As I think about this now, I'm wondering if the image widget hook / callbacks shouldn't go in uc_store.module. The particular setting for products could still go in the product settings form.

• ...
• Profit!

Hope this is helpful... I could've just made the changes myself prior to committing, but wanting to have a little more open/contagious thought process. : )

I wanted to give this an awesome +1, this is great because the acquia latest release ships with lightbox2, which means having lightbox2 work with ubercart means no modules need to be loaded besides ubercart core on top of acquia waaahoooo

thanks for the suggestions, Ryan. They have been incorporated in this latest patch. I wasn't sure what you mean by:

"Also related to the settings, with a radio select list like this, the title can essentially be what your description is w/ no description necessary... i.e. title => 'Image widget to use when a customer clicks on a product image'. Perhaps the description should then become => 'Third party modules offer pop-up support for viewing full sized product images. Ubercart core accommodates Thickbox and Lightbox2.'"

Since I didn't have a description on the last patch's radio dialog. I modified the language a bit though. Also, I had missed using t() there. I made a couple other little changes/fixes to make things work better with the suggestions.

P.S. Thanks for the kind words Tim. :)

after applying this to latest bzr i am getting this

PHP Fatal error:  Call to undefined function uc_store_image_widget_lightbox2() in /Applications/MAMP/htdocs/d6u2/sites/all/modules/ubercart/uc_product/uc_product.module on line 1409

Thanks, Tim... I forgot to rename the callback function when I updated the hook. Here's another patch that fixes that.

Awesome, works much better now, great feature!

Looks good. I think I might actually start using Lightbox2 for my test sites. :)

It is all set in the beta2 release, I have actually switched two live sites over and deleted thickbox.